Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to Groovy 4.0 #8010

Merged
merged 33 commits into from
Oct 7, 2022
Merged

Update to Groovy 4.0 #8010

merged 33 commits into from
Oct 7, 2022

Conversation

timyates
Copy link
Contributor

@timyates timyates commented Sep 15, 2022

This PR switches the Groovy version in core to 4.0.5

  • The internal plugin pulls in an old version of Groovy, so I added a dependency resolution block. This can be removed once the internal plugin is updated for Micronaut 4.0.0
  • Geb doesn't support Groovy 4.x. To re-enable the Geb tests, I have split these (one of them) out into a new module test-groovy-geb which runs with Groovy 3 on the classpath
  • Groovy no longer creates isXXX getters for Boolean fields (it does for boolean) -- this is discussed over here on the Groovy Jira, and a fix has been added to only check isXXX for primitive booleans
  • Groovy can no longer grab a self-signed certificate using the old sun classes. I have fixed this by adding bouncycastle onto the test classpath, so it uses that for the test
  • Groovy 4.x can no longer use the static methods on an interface declared in the class. ie: If an interface declares static void foo(), and Concrete implements Interface then Groovy must call Interface.foo(), not Concrete.foo(). This is an expected change -- I asked in the Groovy JIRA
  • inject-kotlin-test/.../KotlinCompilerTest fails with Groovy 4 as the Note for beans being created are picked up as a catastrophic failure by the test harness. I added a check to ignore Notes, as this is only under test when Groovy is creating a kotlin build with Kapt processing

@sdelamo sdelamo added this to the 4.0.0 milestone Sep 16, 2022
@timyates
Copy link
Contributor Author

I think one of the bugs I had to workaround is https://issues.apache.org/jira/browse/GROOVY-10060

@@ -111,6 +116,11 @@ import jakarta.inject.Singleton
class Config
{
Boolean boolProperty

// TODO: Why do we need this? Groovy bug?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's expected and not a bug, fixed in fd8d885

@timyates timyates marked this pull request as ready for review September 22, 2022 12:06
@timyates timyates requested review from melix and sdelamo and removed request for melix September 22, 2022 12:06
@timyates timyates self-assigned this Sep 22, 2022
@@ -46,6 +46,15 @@ tasks.withType(Jar).configureEach {
preserveFileTimestamps = false
}

configurations.all {
resolutionStrategy.eachDependency { DependencyResolveDetails details ->
if (details.requested.group == 'org.codehaus.groovy') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So while this will work to build and execute this module, this won't be propagated to consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's ok... It's only stopping the micronaut-build internal plugin pulling in the codehaus dependency here

Once micronaut-build is updated, I believe we can get rid of this

tasks.named("test", Test) {
if(JavaVersion.current().majorVersion.toInteger() >= 17) {
jvmArgs(
'--add-opens', 'jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole block is a bit scary!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but without it we get

java.lang.IllegalAccessError: class org.jetbrains.kotlin.kapt3.base.KaptContext (in unnamed module @0x7a4f0f29) cannot access class com.sun.tools.javac.util.Context (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.util to unnamed module @0x7a4f0f29

All the way down until you end up with the above list. I think it's because we're effectively hand-cranking Kapt from Groovy, but I have no proof...


testImplementation libs.geb.spock
testImplementation libs.spock.for.geb
testImplementation "org.codehaus.groovy:groovy-test:3.0.12"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional that it uses an old version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, test-suite-geb runs Geb on Groovy 3 (as there's no Geb that works with Groovy 4 -- yet)

test-suite/build.gradle Outdated Show resolved Hide resolved
@@ -15,7 +15,9 @@ dependencies {
api project(":core-reactive")
api libs.managed.validation

compileOnly libs.managed.gorm
compileOnly(libs.managed.gorm) {
exclude(module: 'groovy')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm, we should figure out why an exclude is needed here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libs.managed.gorm is org.grails:grails-datastore-core:7.3.2 at time of writing which has a compile dependency on Groovy 3

    <dependency>
      <groupId>org.codehaus.groovy</groupId>
      <artifactId>groovy</artifactId>
      <version>3.0.11</version>
      <scope>compile</scope>
    </dependency>

This is just a compile only dependency, but we may be breaking gorm integration until grals release a Groovy 4 version of their libraries that we consume

Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove GORM information from the documentation?

Maybe add it later once there is a version of GORM compatible with Groovy 4.

Or add an Asciidoc callout?

@timyates timyates requested a review from melix September 26, 2022 09:48
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

50.0% 50.0% Coverage
0.0% 0.0% Duplication

@sdelamo sdelamo merged commit ec42b98 into 4.0.x Oct 7, 2022
@sdelamo sdelamo deleted the groovy-4.0 branch October 7, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants